Skip to content

server: add EngineStats endpoint in multitenant setup #143633

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 10, 2025

Conversation

shaikzakiriitm
Copy link
Contributor

@shaikzakiriitm shaikzakiriitm commented Mar 28, 2025

EngineStats endpoint was inaccessible from secondary tenant.

EngineStats endpoint provides statistics of storage layer which can help diagnose myriad of issues critical to database performance.
To address this, we created EngineStatus endpoint in tenant status server. Implementation uses tenant connector to redirect call to system status server on requested kv node. Access to this endpoint is guarded by can_view_node_info capability as this endpoint returns info of all stores on a given node. Updated corresponding unit tests.

Epic: CRDB-38968
Fixes: #110020

Release note: None

@shaikzakiriitm shaikzakiriitm requested review from a team as code owners March 28, 2025 06:22
@shaikzakiriitm shaikzakiriitm requested review from aerfrei and xinhaoz and removed request for a team March 28, 2025 06:22
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@shaikzakiriitm shaikzakiriitm changed the title server,kv,rpc: add EngineStats endpoint in multitenant setup server: add EngineStats endpoint in multitenant setup Apr 4, 2025
Copy link
Contributor

@shubhamdhama shubhamdhama left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left some minor comments above. The rest of the PR looks good to me.

Copy link
Contributor

@cthumuluru-crdb cthumuluru-crdb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple of comments. Otherwise it looks good.

Copy link
Contributor

@cthumuluru-crdb cthumuluru-crdb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple of comments. Otherwise it looks good.

Copy link
Contributor

@shubhamdhama shubhamdhama left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

EngineStats endpoint was inaccessible from secondary tenant.

EngineStats endpoint provides statistics of storage layer which can help
diagnose myriad of issues critical to database performance.
To address this, we created EngineStatus endpoint in tenant status server.
Implementation uses tenant connector to redirect call to system status server
on requested kv node. Access to this endpoint is guarded by `can_view_node_info`
capability as this endpoint returns info of all stores on a given node. Updated
corresponding unit tests.

Epic: CRDB-38968
Fixes: cockroachdb#110020
Part of: CRDB-31226

Release note: None
@shaikzakiriitm
Copy link
Contributor Author

bors r=shubhamdhama,cthumuluru-crdb

@craig
Copy link
Contributor

craig bot commented Apr 10, 2025

👎 Rejected by code reviews

Copy link
Contributor

@cthumuluru-crdb cthumuluru-crdb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you!

@shaikzakiriitm
Copy link
Contributor Author

bors r=shubhamdhama,cthumuluru-crdb

@craig
Copy link
Contributor

craig bot commented Apr 10, 2025

@craig craig bot merged commit 17e9fdd into cockroachdb:master Apr 10, 2025
24 checks passed
@shaikzakiriitm shaikzakiriitm deleted the 110020 branch April 10, 2025 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

server: the "enginestats" status API endpoint should work with secondary tenants with sufficient capability
4 participants